Refactor AutoIncrementTracker into generitc type with interfaces - #11337
Refactor AutoIncrementTracker into generitc type with interfaces#11337nicktobey wants to merge 11 commits into
Conversation
|
@nicktobey DOLT
|
|
@nicktobey DOLT
|
|
@nicktobey DOLT
|
|
@coffeegoddd DOLT
|
|
@nicktobey DOLT
|
Hydrocharged
left a comment
There was a problem hiding this comment.
LGTM! Just some minor nitpicks
| mm: mutexmap.NewMutexMap(), | ||
| init: make(chan struct{}), | ||
| cancelInit: make(chan struct{}), | ||
| func NewSequenceTrackerFromRoots[ |
There was a problem hiding this comment.
In general, I think it would be worthwhile to add function and struct comments, in case someone else needs to modify these in the future.
|
|
||
| if seq > newHighestValue { | ||
| newHighestValue = seq | ||
| if newHighestValue == nil { |
There was a problem hiding this comment.
It's a nitpick, but technically for Doltgres, negative sequences aren't the highest values, so I'd give this a different name.
zachmu
left a comment
There was a problem hiding this comment.
Overall looks good, one pretty annoying comment about using doltdb.TableName in these interface.
| DropTable(ctx *sql.Context, relation string, wses ...*doltdb.WorkingSet) error | ||
| // InitWithRoots fills the SequenceTracker with values pulled from each root in order. | ||
| InitWithRoots(ctx context.Context, roots ...doltdb.Rootish) error | ||
| Close() |
There was a problem hiding this comment.
Should document the contract and expectations for close
| ) | ||
|
|
||
| // SequenceTrackerBase is the non-generic base interface for SequenceTracker | ||
| // It is useful for establishing an upper bound on type parameters in some circumstances. |
There was a problem hiding this comment.
Is this the best way to do this? Don't want you to rat-hole too deep but this is a little odd.
| StateType sequences.SequenceState[StateType, ValueType], | ||
| ValueType comparable, | ||
| ] interface { | ||
| GetRelation(ctx context.Context, root doltdb.RootValue, tName doltdb.TableName) (relation RelationType, resolvedName string, found bool, err error) |
|
@nicktobey DOLT
|
|
@nicktobey DOLT
|
The purpose of this PR is to enable Doltgres to create a global state for Sequences that behaves similarly to Dolt's global state for auto increment columns. In order to allow for code reuse, we have to do the following:
(Bikeshedding note: I use the term "Relation" to describe a top-level object in the database, and encompasses both tables and root objects. I'm open to alternative names.)
The new interfaces can be found in go/libraries/doltcore/sqle/globalstate/sequences/state.go
The common logic is in the SequenceTracker type, which has an interface in go/libraries/doltcore/sqle/globalstate/sequence_tracker.go and an implementation in go/libraries/doltcore/sqle/dsess/sequence_tracker.go
AutoIncrementTracker is now a type alias for a specialization of SequenceTracker.
Finally, while GlobalState previously only held an AutoIncrementTracker, it now contains a map from arbitrary keys to SequenceTrackers. This will allow Doltgres to add an additional SequenceTracker for global tracking of Doltgres sequences.
There are a couple places in this PR that are TODOs, because they are corner cases that aren't reachable for AutoIncrement but are reachable for Doltgres sequences. They mostly concern how to handle when branches contain incompatible Sequences. I haven't decided exactly how this should be handled, and this PR will not be merged until we know what to do in that case, but it shouldn't affect the review of the rest of the PR.